Skip to content

fix(operator): inject THV_SESSION_REDIS_PASSWORD for MCPServer#5286

Merged
jhrozek merged 2 commits into
stacklok:mainfrom
dallinstevens:fix/mcpserver-redis-password-injection
May 19, 2026
Merged

fix(operator): inject THV_SESSION_REDIS_PASSWORD for MCPServer#5286
jhrozek merged 2 commits into
stacklok:mainfrom
dallinstevens:fix/mcpserver-redis-password-injection

Conversation

@dallinstevens
Copy link
Copy Markdown
Contributor

Problem

`MCPServer.spec.sessionStorage.passwordRef` is accepted by the CRD schema, and the runner reads `THV_SESSION_REDIS_PASSWORD` (see `pkg/transport/session/redis_config.go`), but the operator never wires the two together. `populateScalingConfig` in `cmd/thv-operator/controllers/mcpserver_runconfig.go` copies only `Address`/`DB`/`KeyPrefix` into RunConfig, and no env var is appended to the proxy runner container.

Result: any `MCPServer` pointed at a password-protected Redis fails on startup with:

```
{"level":"ERROR","msg":"error executing command","error":"failed to create Redis session storage: failed to connect to redis: NOAUTH Authentication required."}
```

reproducible at v0.27.2 and on `main` as of this PR's base.

Why this was missing

PR #4368 (which introduced MCPServer runconfig wiring) explicitly noted:

the Redis password is intentionally excluded and injected as a pod env var instead

The env-var injection then landed for `VirtualMCPServer` in #4215 via `buildRedisPasswordEnvVar` — but the parallel MCPServer change wasn't made. From the user's side, the field looks supported (CRD validation passes, runner accepts the value) but silently no-ops at runtime.

Fix

Mirror the VirtualMCPServer pattern:

  1. `mcpserver_controller.go` — new helper `buildMCPServerRedisPasswordEnvVar` that returns `[]corev1.EnvVar` with a single `SecretKeyRef`-backed `THV_SESSION_REDIS_PASSWORD` entry when `sessionStorage.provider == "redis"` and `passwordRef` is set; returns `nil` otherwise.
  2. Append its output to the proxy container's env slice in the MCPServer reconciler, immediately before `resourceOverrides.proxyDeployment.env` so user-supplied overrides still win on name collisions.
  3. `mcpserver_replicas_test.go` — add `TestBuildMCPServerRedisPasswordEnvVar` mirroring `TestBuildRedisPasswordEnvVar` for VirtualMCPServer (nil storage, memory provider, redis without passwordRef, redis with passwordRef).

Scope

Test plan

  • `go build ./cmd/thv-operator/...`
  • `go test -count=1 ./cmd/thv-operator/controllers/` — all green (3.7s)
  • New `TestBuildMCPServerRedisPasswordEnvVar` covers four cases (nil, memory, redis-no-passwordRef, redis-with-passwordRef) and asserts the env var uses `SecretKeyRef` (no plaintext `Value`).
  • Integration verification: deployed locally against an MCPServer running with `replicas: 2` against a password-protected Redis; proxy connects cleanly.

Related

@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 14, 2026
MCPServer.spec.sessionStorage.passwordRef is accepted by the CRD
schema and the runner reads THV_SESSION_REDIS_PASSWORD from
pkg/transport/session/redis_config.go, but the operator never bridged
the two: populateScalingConfig in mcpserver_runconfig.go copies only
Address/DB/KeyPrefix into RunConfig, and no env var is added to the
proxy runner container. Result: any MCPServer that points its session
storage at a password-protected Redis fails on connect with
"NOAUTH Authentication required".

PR stacklok#4368 (which introduced the runconfig wiring) explicitly noted in
its summary that "the Redis password is intentionally excluded and
injected as a pod env var instead". The env-var injection landed for
VirtualMCPServer in stacklok#4215 / buildRedisPasswordEnvVar but the parallel
MCPServer change was never made.

This change closes that gap by mirroring the VirtualMCPServer pattern:

- Add buildMCPServerRedisPasswordEnvVar in mcpserver_controller.go
  which returns []corev1.EnvVar with a single SecretKeyRef-backed
  THV_SESSION_REDIS_PASSWORD entry when sessionStorage.provider ==
  "redis" and passwordRef is set; returns nil otherwise.
- Append the helper's output to the proxy container's env slice in
  the MCPServer reconciler, immediately before user-supplied
  resourceOverrides.proxyDeployment.env so user overrides can still
  win on name collisions.
- Add TestBuildMCPServerRedisPasswordEnvVar mirroring
  TestBuildRedisPasswordEnvVar for VirtualMCPServer (nil storage,
  memory provider, redis without passwordRef, redis with passwordRef).
- No CRD changes — the schema already supports passwordRef.

Tested:
- go build ./cmd/thv-operator/... succeeds
- go test ./cmd/thv-operator/controllers/ all green (3.7s)
The previous Tests / Test Go Code job (76080809592) was killed by
GitHub Actions infrastructure mid-run ("task: Signal received:
\"terminated\"", "runner has received a shutdown signal") ~5.5 min
into 'task test-coverage'. No test failures surfaced in the truncated
logs. Push an empty commit to retrigger.
@dallinstevens dallinstevens force-pushed the fix/mcpserver-redis-password-injection branch from 54d4187 to a6af4da Compare May 14, 2026 22:17
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 14, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.43%. Comparing base (17451d1) to head (a6af4da).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5286      +/-   ##
==========================================
+ Coverage   68.37%   68.43%   +0.05%     
==========================================
  Files         619      619              
  Lines       63318    63335      +17     
==========================================
+ Hits        43293    43342      +49     
+ Misses      16794    16758      -36     
- Partials     3231     3235       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhrozek
Copy link
Copy Markdown
Contributor

jhrozek commented May 19, 2026

thanks for the PR @dallinstevens and I apologise for the delay in having this reviewed

@jhrozek jhrozek merged commit 6d81df8 into stacklok:main May 19, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants